Fix VPC restart with multi-CIDR networks: handle comma-separated CIDR… #12622
Conversation
… in NetworkVO.equals() When a network has multiple CIDRs (e.g. '192.168.2.0/24,160.0.0.0/24'), NetworkVO.equals() passes the raw comma-separated string to NetUtils.isNetworkAWithinNetworkB() which expects a single CIDR, causing 'cidr is not formatted correctly' error during VPC restart with cleanup=true. Extract only the first CIDR value before passing to NetUtils.
|
@blueorangutan package |
|
@blueorangutan package |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12622 +/- ##
=========================================
Coverage 17.90% 17.90%
+ Complexity 16094 16093 -1
=========================================
Files 5938 5938
Lines 532864 532866 +2
Branches 65192 65192
=========================================
+ Hits 95392 95395 +3
Misses 426793 426793
+ Partials 10679 10678 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
There was a problem hiding this comment.
Pull request overview
Fixes VPC restart failures for multi-CIDR networks by ensuring NetworkVO.equals() does not pass a raw comma-separated CIDR list into NetUtils.isNetworkAWithinNetworkB(), which expects a single CIDR.
Changes:
- Normalize
cidrvalues inNetworkVO.equals()by extracting the first CIDR from comma-separated strings before comparing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return NetUtils.isNetworkAWithinNetworkB( | ||
| com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(cidr), | ||
| com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(that.cidr)); |
There was a problem hiding this comment.
equals is using NetUtils.isNetworkAWithinNetworkB(...), which is not symmetric (A within B does not imply B within A). That means NetworkVO.equals can violate the Java equals contract, and it is also inconsistent with hashCode() (which is based only on id). Consider switching equals/hashCode to be id-based like other VOs, or (if CIDR-based equality is truly intended) make the comparison symmetric and update hashCode accordingly (e.g., based on the same normalized CIDR(s) and trafficType).
| return NetUtils.isNetworkAWithinNetworkB( | ||
| com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(cidr), | ||
| com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(that.cidr)); |
There was a problem hiding this comment.
Minor maintainability: getFirstValueFromCommaSeparatedString(...) is computed twice inline. Assign the normalized CIDRs to local variables before calling isNetworkAWithinNetworkB to avoid repeated splitting/trimming and make debugging/logging easier.
| return NetUtils.isNetworkAWithinNetworkB( | |
| com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(cidr), | |
| com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(that.cidr)); | |
| final String normalizedCidr = com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(cidr); | |
| final String normalizedThatCidr = com.cloud.utils.StringUtils.getFirstValueFromCommaSeparatedString(that.cidr); | |
| return NetUtils.isNetworkAWithinNetworkB(normalizedCidr, normalizedThatCidr); |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16776 |
4 participants
#12621
… in NetworkVO.equals()
When a network has multiple CIDRs (e.g. '192.168.2.0/24,160.0.0.0/24'), NetworkVO.equals() passes the raw comma-separated string to NetUtils.isNetworkAWithinNetworkB() which expects a single CIDR, causing 'cidr is not formatted correctly' error during VPC restart with cleanup=true.
Extract only the first CIDR value before passing to NetUtils.
Description
This PR...
Types of changes
Bug fix (non-breaking change which fixes an issue)
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
on different VLANs assigned to the same VPC
How did you try to break this feature and the system with this change?